Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BugFix] Overhaul async request cancellation #7111

Merged
merged 7 commits into from
Aug 7, 2024

Conversation

njhill
Copy link
Member

@njhill njhill commented Aug 3, 2024

There are a number of problems currently with how request cancellation works upon client disconnection in the openai api server front-end and AsyncLLMEngine:

  • Abort logic is missing for streaming chat requests - these continue to run after the client disconnects
  • For multi-prompt completion requests, only one of the n running sequences actually gets aborted
  • Cancelled requests that are queued will still be pre-filled before getting aborted
  • Some of the existing async generator logic may be contributing to clean-shutdown garbage collection issues

This is a problem for production resilience and has a compounding affect when the server is overloaded and client requests time-out, with the server continuing to do useless work.

This PR reworks how the cancellation is propagated to make it more robust and consistent:

  • Use native asyncio task/generator cancellation propagation as much as possible. This includes making use of explicitly cancellable async generators (via asyncio.aclose()) rather than async iterators in most cases
  • Re-implement merge_async_iterators to encapsulate polling for disconnection, even before any results have been produced (while the request is queued)
  • Add equivalent iterate_with_cancellation function used for the single-prompt request cases
  • In AsyncLLMEngine differentiate between cancelled requests and those that finish normally (generator returned from the generate() will now raise a CancelledError in the former case)
  • Don't abort requests that have already completed normally. This avoids some redundant work in the engine to look up sequence groups that have already gone

I also plan to add some new tests to cover these various cases.

Copy link

github-actions bot commented Aug 3, 2024

👋 Hi! Thank you for contributing to the vLLM project.
Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which consists a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of default ones by unblocking the steps in your fast-check build on Buildkite UI.

Once the PR is approved and ready to go, please make sure to run full CI as it is required to merge (or just use auto-merge).

To run full CI, you can do one of these:

  • Comment /ready on the PR
  • Add ready label to the PR
  • Enable auto-merge.

🚀

@robertgshaw2-neuralmagic
Copy link
Collaborator

Nice

@njhill njhill added the ready ONLY add when PR is ready to merge/full CI is needed label Aug 5, 2024
Copy link
Member

@DarkLight1337 DarkLight1337 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, sorry for keeping you waiting.

@DarkLight1337 DarkLight1337 merged commit 9a3f49a into vllm-project:main Aug 7, 2024
51 checks passed
@njhill njhill deleted the rework-aborts branch August 7, 2024 13:07
njhill added a commit to njhill/vllm that referenced this pull request Aug 7, 2024
vllm-project#7111 made a change to the merge_async_iterators utils function to add an is_cancelled arg. It would be good for this new arg to be optional to retain backwards compatibility for other server front-ends that might already be using this utility function.
njhill added a commit to njhill/vllm that referenced this pull request Aug 7, 2024
vllm-project#7111 made a change to the merge_async_iterators utils function to add an is_cancelled arg. It would be good for this new arg to be optional to retain backwards compatibility for other server front-ends that might already be using this utility function.
njhill added a commit to njhill/vllm that referenced this pull request Aug 9, 2024
Follow-on from vllm-project#7111, avoid unecessary enqueuing a final message after an exception and avoid aborting requests in the engine that were never started.
sfc-gh-mkeralapura pushed a commit to sfc-gh-mkeralapura/vllm that referenced this pull request Aug 12, 2024
kylesayrs pushed a commit to neuralmagic/vllm that referenced this pull request Aug 17, 2024
fialhocoelho pushed a commit to opendatahub-io/vllm that referenced this pull request Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants